Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail explanatory assertion group on empty assertionCreator #938

Conversation

wordhou
Copy link
Collaborator

@wordhou wordhou commented Jun 11, 2021

Resolves #933. Whenever an assertionCreator is taken as an argument of an expectation, an explanatory group is created that describes the assertions created. If no assertions are created, then an empty lambda failure hint is given. This PR makes the explanatory group fail when the empty lambda failure hint is given, and then removes the createEmptyAssertionHintIfNecessary from the assertion creators.


I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@wordhou wordhou requested a review from robstoll as a code owner June 11, 2021 16:49
// not using addAssertionsCreatedBy on purpose so that we don't append a failing assertion
collectingExpect.assertionCreatorOrNull()
if(collectingExpect.getAssertions().isEmpty()) {
it.collectAssertions(container, None, assertionCreatorOrNull).failing
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to make collectAssertions(...) fail the explanatory group when the assertionCreator doesn't create any assertions?

Copy link
Owner

@robstoll robstoll Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a reasonable thing. That an explanatory group can fail is rather a new thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize I'm not actually sure how to modify the builder to accomplish this.

I'm looking at AssertionOptionExplanatoryExtensions.kt and I'm not sure how to get the extension method .collectAssertions to produce a failing final step.

My naive attempt:

fun <T, G : ExplanatoryAssertionGroupType, R> AssertionsOption<G, R>.collectAssertions(
    container: AssertionContainer<*>,
    maybeSubject: Option<T>,
    assertionCreator: Expect<T>.() -> Unit
): R {
    val collectingExpect = CollectingExpect<T>(None, container.components)
    // not using addAssertionsCreatedBy on purpose so that we don't append a failing assertion
    collectingExpect.assertionCreator()
    return withAssertions(container.collectForCompositionBasedOnSubject(maybeSubject, assertionCreator))
        .let {
            if(collectingExpect.getAssertions().isEmpty()) it.failing
            else it
        }
}

has issues because the return type is the generic type R and it doesn't recognize .failing. I'm not really understanding here how the builder pattern is implemented here with the generic AssertionsOption<G,R> type.

Copy link
Owner

@robstoll robstoll Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to restrict R with an upper bound of ExplanatoryGroup.FinalStep.kt in AssertionOptionExplanatoryExtensions.kt.
So like the following:

fun <T, G : ExplanatoryAssertionGroupType, R : ExplanatoryGroup.FinalStep> AssertionsOption<G, R>.collectAssertions(...

Copy link
Collaborator Author

@wordhou wordhou Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above type signature the compiler complained about two things:

  • It expects the method to return R but the inferred type is ExplanatoryGroup.FinalStep
  • Elsewhere in the code, in 4 other places, it complains about the upper bound R : ExplanatoryGroup.FinalStep, saying ExplanatoryAssertionGroupFinalStep isn't a subtype of ExplanatoryGroup.FinalStep. I think the problem is that the type options withDefaultType, withWarningType, withInformationType and withType all have a type signature AssertionsOption<T, ExplanatoryAssertionGroupFinalStep>, at least for now. I saw your notes about replacing ExplanatoryAssertionGroupFinalStep with ExplanatoryGroup.FinalStep before version 1.0.0.

So I had to use the following to satisfy the compiler.

fun <T, G : ExplanatoryAssertionGroupType, R : ExplanatoryAssertionGroupFinalStep> AssertionsOption<G, R>.collectAssertions(
    container: AssertionContainer<*>,
    maybeSubject: Option<T>,
    assertionCreator: Expect<T>.() -> Unit
) : ExplanatoryAssertionGroupFinalStep {
  // ...
}

However this generates a warning on compilation. Am I approaching this in the right way? Do we suppress/ignore the warning or do something else?

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #938 (568a1bf) into master (e73e465) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   91.34%   91.40%   +0.06%     
==========================================
  Files         429      429              
  Lines        4299     4295       -4     
  Branches      219      218       -1     
==========================================
- Hits         3927     3926       -1     
+ Misses        323      320       -3     
  Partials       49       49              
Flag Coverage Δ
bbc 80.79% <100.00%> (-0.02%) ⬇️
bc 80.70% <100.00%> (-0.01%) ⬇️
current 87.63% <100.00%> (+0.05%) ⬆️
current_windows 86.70% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...creators/impl/InAnyOrderEntriesAssertionCreator.kt 97.29% <ø> (-0.53%) ⬇️
...collectors/AssertionsOptionExplantoryExtensions.kt 100.00% <100.00%> (ø)
...ium/api/infix/en_GB/chronoLocalDateExpectations.kt 100.00% <0.00%> (ø)
.../api/infix/en_GB/chronoZonedDateTimeExpectation.kt 100.00% <0.00%> (ø)
...api/infix/en_GB/chronoLocalDateTimeExpectations.kt 100.00% <0.00%> (ø)
...pi/fluent/en_GB/chronoZonedDateTimeExpectations.kt 100.00% <0.00%> (ø)
...logic/impl/DefaultChronoZonedDateTimeAssertions.kt 87.50% <0.00%> (+7.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e73e465...568a1bf. Read the comment docs.

@robstoll
Copy link
Owner

@wordhou sorry, I am quite busy at the moment, going to look into it hopefully soon

@robstoll robstoll merged commit 8df4707 into robstoll:master Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty assertionCreator lambda warning is added twice
2 participants